Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added type annotation to the data_analysis_tools #63

Merged
merged 5 commits into from
Oct 14, 2019
Merged

Added type annotation to the data_analysis_tools #63

merged 5 commits into from
Oct 14, 2019

Conversation

Abelarm
Copy link
Contributor

@Abelarm Abelarm commented Oct 13, 2019

Issue: #55

  • Added type annotation
  • Tested

Output test:
`mypy ellipsometry_analysis.py

ellipsometry_analysis.py:1: error: No library stub file for module 'numpy'
ellipsometry_analysis.py:1: note: (Stub files are from https://github.com/python/typeshed)
ellipsometry_analysis.py:2: error: No library stub file for module 'matplotlib.pyplot'
ellipsometry_analysis.py:2: error: No library stub file for module 'matplotlib'
ellipsometry_analysis.py:4: error: Cannot find module named 'solcore.absorption_calculator'
ellipsometry_analysis.py:4: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
ellipsometry_analysis.py:5: error: No library stub file for module 'scipy.optimize'
ellipsometry_analysis.py:391: error: Cannot find module named 'solcore.absorption_calculator.dielectric_constant_models'
Found 6 errors in 1 file (checked 1 source file)
`

all the errors don't depend on the typing added

@dalonsoa
Copy link
Collaborator

There re two problems: Solcore has so few annotations that it will inevitably fail any attempt to go through mypy, at least for now. Unless all things ellipsometry_annalysis.py depends on are annotated, it won't work.

Besides, even if that is done, neither Matplotlib, Scipy nor Numpy will pass mypy checks anyway because they don't have stud files with proper type annotations! The simplest solution, at least for now, is to create a mypy.ini file in the root repo directory with instructions to ignore those errors:

[mypy]
ignore_missing_imports = True
[mypy-setup]
ignore_errors = True

Still, this will not solve the first problem, which requires Solcore to be properly annotated. Please, feel free to add this mypy.ini file.

Let me know when you are done clicking on request rerview above :)

Many thanks for dealing with this!

@Abelarm
Copy link
Contributor Author

Abelarm commented Oct 14, 2019

added the mypy, where is it request review?

@dalonsoa
Copy link
Collaborator

Top right of this PR screen, where it says "Reviwers". Anyway, I'm having a look at it, now.

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small change when annotating parameters that can take the value None.

solcore/data_analysis_tools/ellipsometry_analysis.py Outdated Show resolved Hide resolved
solcore/data_analysis_tools/ellipsometry_analysis.py Outdated Show resolved Hide resolved
@Abelarm Abelarm requested a review from dalonsoa October 14, 2019 11:39
@dalonsoa dalonsoa merged commit 345af17 into qpv-research-group:devel Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants